Add GOOGLE_INTERNAL_GSM_NAMESPACE and GOOGLE_INTERNAL_GSM_DEPLOYMENT to node metadata, #68
Conversation
There was a problem hiding this comment.
Is there a reason we don't want to use TRAFFIC_DIRECTOR_CLIENT_ENVIRONMENT ? gkeDeployment in particular is already plumbed through that. That is disabled currently, but there's some movement that seems may enable it. CC @arvindbr8
node metadata. This will be used for apply authorization policies to deployments. Used only in CSM Gateway API cluster.
karthikbox
force-pushed
the
deployment-name
branch
from
787c12c to
2e582d4
Compare
There was a problem hiding this comment.
Could you please resolve ejona's [comment]? (#68 (review)). Karthik mentioned that these labels exists in Envoy.
| vpcNetworkName: "thedefault", | ||
| ip: "10.9.8.7", | ||
| zone: "uscentral-5", | ||
| metadataLabels: map[string]string{ |
There was a problem hiding this comment.
The functionality test for adding metadataLabels are already being tested. If you still would like to have another test that adds these exact labels, can we split them to another test?
| @@ -48,6 +48,7 @@ var ( | |||
| gkePodName = flag.String("gke-pod-name-experimental", "", "GKE pod name to use, instead of reading it from $HOSTNAME or /etc/hostname file. This flag is EXPERIMENTAL and may be changed or removed in a later release.") | |||
| gkeNamespace = flag.String("gke-namespace-experimental", "", "GKE namespace to use. This flag is EXPERIMENTAL and may be changed or removed in a later release.") | |||
| gkeLocation = flag.String("gke-location-experimental", "", "the location (region/zone) of the GKE cluster, instead of retrieving it from the metadata server. This flag is EXPERIMENTAL and may be changed or removed in a later release.") | |||
| gkeDeployment = flag.String("gke-deployment-experimental", "", "GKE deployment to use. This flag is EXPERIMENTAL and may be changed or removed in a later release.") | |||
There was a problem hiding this comment.
nit: Should we add more info to the usage text?
There was a problem hiding this comment.
Why "deployment"? What about bare pods, bare replica sets, and daemon sets? Are these not supported? Or is "deployment"? Or is this more abstract and not referring to the Kubernetes Deployment resource?
There was a problem hiding this comment.
this behavior could be possible by passing in the k,v to --node-metadata
./td-grpc-bootstrap --node-metadata GOOGLE_INTERNAL_GSM_NAMESPACE=foo --node-metadata GOOGLE_INTERNAL_GSM_DEPLOYMENT=bar
it's worth mentioning that setting this flag will overwrite the value passed to --node-metadata
i did consider that however but we'd still have an inconsistency with envoy node metadata. currently, none of csm envoys set traffic_director_client_environment but they do set the GOOGLE_INTERNAL_GSM_NAMESPACE and GOOGLE_INTERNAL_GSM_DEPLOYMENT (there are other keys such as this in envoy at the top level in node metadata but i left them out as they're not needed at the moment). This logic is already baked into TD control plane and is used in google cloud 1n authz policy (preview) as well as other GA features (istio api for example) for applying the policy to workloads. when i proposed TRAFFIC_DIRECTOR_CLIENT_ENVIRONMENT originally, it was only meant to make it easier for users to search control plane logs. not for applying policies on. there's no strong reason not to use this for policies as well but at this point its even more work to add this to envoy and teach TD to look for these nested labels. on the other hand, may be its good to keep these labels separate as they're used for different pruposes ? overall, this is still a net positive. as we get 1n authz support for grpc clients (like envoys) but comes at a small cost of internal redundant info. |
|
I don't think I'm the right person to decide this. This seems like a CSM decision. |
|
I think would be nice to also get @gnossen 's review here |
| @@ -48,6 +48,7 @@ var ( | |||
| gkePodName = flag.String("gke-pod-name-experimental", "", "GKE pod name to use, instead of reading it from $HOSTNAME or /etc/hostname file. This flag is EXPERIMENTAL and may be changed or removed in a later release.") | |||
| gkeNamespace = flag.String("gke-namespace-experimental", "", "GKE namespace to use. This flag is EXPERIMENTAL and may be changed or removed in a later release.") | |||
| gkeLocation = flag.String("gke-location-experimental", "", "the location (region/zone) of the GKE cluster, instead of retrieving it from the metadata server. This flag is EXPERIMENTAL and may be changed or removed in a later release.") | |||
| gkeDeployment = flag.String("gke-deployment-experimental", "", "GKE deployment to use. This flag is EXPERIMENTAL and may be changed or removed in a later release.") | |||
|
We found a faster approach by using the exiting flag |
This will be used for apply authorization policies to deployments. Used only in CSM Gateway API cluster.